fix: close the tls server config a listen loop built - #688
Merged
Conversation
every std tls server built a config from its cert and key pem files and never closed it, so the certificate pem and the private key der stayed in the tls registry for the life of the process. a program that restarts its listeners in place paid that cost once per restart. the ownership rule is the same one the client side settled on: whoever builds a config closes it. tls.listen records the handle against the listening socket, but that is a borrow, not a transfer, so Listener.close() is right to give back only the borrow and the three creators are the ones that owe a close -- App.listen_tls, listen_h2_tls, and listen_h2_tls_streaming. what makes a server different is when. an accept loop spawns a task per connection, and that task reads the certificate and the key out of the registry when its own handshake reaches them, which can be a whole handshake timeout after the accept returned. so the close goes after the drain, not in a defer that would also fire on the give-up path where nothing drains. the bind-failure exit closes too: nothing has borrowed the config at that point. adds tls.open_server_configs() -- how many server configs still hold a certificate -- so a leak is observable from outside the module, and documents the server half of the rule in docs/tls.md, which until now claimed the listener owned the config.
the change it covers closes a config that spawned handshake tasks read from, so a use after close is the failure it carries. valgrind is what would see one.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Every std tls server built a config from its cert and key pem files and never closed it, so the certificate pem and the private key der stayed in the tls registry for the life of the process. A program that restarts its listeners in place paid that cost once per restart.
The ownership rule is the one the client side already settled on: whoever builds a config closes it.
tls.listenrecords the handle against the listening socket, but that is a borrow rather than a transfer, soListener.close()is right to give back only the borrow, and the three creators are the ones that owe a close —App.listen_tls,listen_h2_tls, and its streaming twin.What makes a server different is the timing. An accept loop spawns a task per connection, and that task reads the certificate and the key out of the registry when its own handshake reaches them, which can be a whole handshake timeout after the accept that spawned it returned.
native_server_handshakeblocks on the client hello first and only callsnative_server_certificate_chainandnative_server_private_keyafter it arrives, so the gap between "the accept loop stopped" and "this connection still needs the key" is up to the 10s handshake timeout. The close therefore goes aftershutdown.drain_default()rather than into adefer, because a defer would also fire on the give-up path, where nothing drains and a task can be mid-handshake. The bind-failure exit closes as well: nothing has borrowed the config at that point, and a server retrying a taken port would otherwise leak a certificate and a key per attempt.Two things came out of the investigation that were worth fixing alongside.
docs/tls.mdclaimed the listener owned its server config and thatListener.close()released it, which is not what the code does and is a plausible reason the three creators never closed; that section now carries the server rule and the ordering, with an example. Andtls.open_server_configs()is new — it counts the server configs still holding a certificate, which makes this class of leak observable from outside the module and is what the new test asserts on.The give-up path (
back_off_after_accept_failurepropagating after a run of accept failures) deliberately still leaves the config open, with a comment saying so. There is no drain on that path to wait behind, and turning in-flight handshakes into failures is worse than one open config on a path that is already ending the server.what was tested
The new
tests/cases/test_tls_server_config_release.pithruns a realApp.listen_tlsand proves both halves of the rule in one pass. It opens a tcp connection, waits forshutdown.inflight()to confirm the server has taken it, callsshutdown.request(), and only then sends the client hello — so the handshake happens strictly inside the drain. It asserts the handshake succeeds, that the config is still open while that connection is served, that the request gets a 200, and thatopen_server_configs()is back to zero oncelisten_tlsreturns. Nothing waits on a fixed sleep; the connect retries until the listener is bound and the ordering is read offshutdown.inflight().The test was falsified three ways against deliberately broken versions of
listen_tls, so it is known to fail when the fix is absent or misplaced:open server configs after: 1handshake after the shutdown request: false,unexpected eofIt matches its golden output under os threads, green with default workers, and green with a single worker.
Two claims behind the ordering were measured rather than argued. A probe confirmed pith evaluates the return expression before firing a
defer, sodeferafterreturn shutdown.drain_default()would run post-drain; the explicit form is used anyway, because the give-up path needs to be excluded. A second probe showedshutdown.drain_default()does not wait for a task that has been spawned but has not yet reachedshutdown.enter()— it returns 0 immediately. That gap does not endanger this change (such a task has not read the listener's config either, and fails cleanly on the mapping the accept loop already dropped), but it is a real pre-existing hole in the drain and is written up below.make run-regressions-onlyis 329/329.make memcheckis clean, with the new case added toMEMCHECK_CASES— it is the only test that drives a tls listen loop through a full shutdown, which is exactly where a use after close would land.pith test std/net/tls.pithis 9/9, including a new "closing a server config empties every registry map it wrote to" that mirrors the existing client-side one.noticed, not fixed
shutdown.drain_default()can return before a connection it should have waited for. Theenter()that makes a connection visible to the drain runs inside the spawned task, so a connection accepted in the last moments before a shutdown can be dropped without the drain ever counting it. Movingenter()into the accept loop, before thespawn, would close it. Separately, the accept loops release the listener's config borrow before draining, so a task that has entered but has not yet read the mapping loses its handshake during a shutdown; the window is a few non-blocking instructions wide, and an attempt to observe it did not, so it is left alone here rather than changed on argument.